Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RN batching and effect behavior #1444

Merged
merged 8 commits into from
Nov 6, 2019
Merged

Fix RN batching and effect behavior #1444

merged 8 commits into from
Nov 6, 2019

Conversation

markerikson
Copy link
Contributor

@markerikson markerikson commented Nov 5, 2019

Fixes #1313 and fixes #1437 (hopefully!).

Per facebook/react#14927 , useLayoutEffect() warns in SSR. We tried to feature-detect that by checking if window exists, and if so, falling back to useEffect:

const useIsomorphicLayoutEffect =
typeof window !== 'undefined' ? useLayoutEffect : useEffect

In #1283 , that was found to be insufficient if someone polyfilled window, so the check was made more explicit:

const useIsomorphicLayoutEffect =
typeof window !== 'undefined' &&
typeof window.document !== 'undefined' &&
typeof window.document.createElement !== 'undefined'
? useLayoutEffect
: useEffect

We then duplicated that over into useSelector as well.

Unfortunately, it looks like that feature detection breaks in React Native, and we've been falling back to useEffect the whole time. This has led to subtle timing bugs popping up, and we (I) ignored the issue reports largely because we couldn't reproduce them in a web environment. In addition, while several folks said that something had broken with connect between 7.0.3 and 7.1.0, I only looked at the one tiny change inside of connect itself, not at the useIsomorphicLayouteffect change, and didn't catch that all the reports were in RN projects.

This PR hopefully fixes all those, by:

@netlify
Copy link

netlify bot commented Nov 5, 2019

Deploy preview for react-redux-docs ready!

Built with commit 1f93e2e

https://deploy-preview-1444--react-redux-docs.netlify.com

Copy link

@MingruiZhang MingruiZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good to us, we can work out a babel plugin to apply native.js for our renderer. Appreciate the fix

@markerikson
Copy link
Contributor Author

@MingruiZhang : what renderer are you using?

@markerikson
Copy link
Contributor Author

Okay, let's go with this.

@aleclarson
Copy link

I just published react-layout-effect with your solution, so it can be shared between libraries.

https://www.npmjs.com/package/react-layout-effect

I'll gladly add you as a maintainer if you want to use it in react-redux.

albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
* Add React Native deps for testing

* Add a Jest config to run tests in an RN environment

* Add initial RN tests

* Add test for RN batch export

* Add jest-native testing assertions

* Extract useIsomorphicLayoutEffect utility

* Add additional RN batching-related tests

* 7.1.2-alpha.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants